Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the trait bound Arc<K>: Borrow<Q> to K: Borrow<Q> (sync and future caches) #167

Merged
merged 7 commits into from
Jul 16, 2022

Conversation

tatsuya6502
Copy link
Member

This PR changes the trait bound Arc<K>: Borrow<Q> to K: Borrow<Q> for the following methods of sync and future caches.

  • contains_key
  • get
  • invalidate

This fixes #163 for sync and future caches. Now their methods will accept &[u8] as the key &Q when K is Vec<u8>.

Change it for `contains_key`, `get` and `invalidate` of `sync` and
`future` caches.
@tatsuya6502 tatsuya6502 self-assigned this Jul 12, 2022
@tatsuya6502 tatsuya6502 added the bug Something isn't working label Jul 12, 2022
@tatsuya6502 tatsuya6502 added this to the v0.9.1 milestone Jul 12, 2022
Fix the unit tests so that they will compile with the MSRV 1.51.
@JayKickliter
Copy link

Thanks for this. Just to be super clear: I used K: Vec<u8> and Q: &[u8] as my particular example, and wouldn't want to limit it to just that use case. That said, this PR seems to do the expected thing that Rust's std collections do.

@JayKickliter
Copy link

This fixed my use-case https://github.com/JayKickliter/cream/pull/1/files#diff-28b0ccc836e6e883168b88ebb6f39508969ece5516e5f81458abd33592b12430

- Revert the previous change on the `key` of `cht::map::bucket::Bucket<K, V>`
  (Changed back from `Arc<K>` to `K`).
- Modify cht's read methods to take an `eq` closure instead of `key: &Q`. This
  provides enough flexibility to the caller to compare `Arc<K>` with `&Q`.
@tatsuya6502
Copy link
Member Author

@JayKickliter

I found a better solution and I updated this branch. The old solution required the key of the internal hash table should be always wrapped in an Arc. But there is one hash table usage in moka that I did not want to wrap the key with Arc:

8488268#diff-3df6f2ad1c01aeaaff49319246d71a65d7fd5ac8ae5bcd0c32a912ba38228f65L263-R263

-        (cht_key, hash)
+        (Arc::new(cht_key), hash)

(I wanted to revert the above change)

The new solution does not require it, so I reverted the above change and I am happy now.

I tried cream's jsk/bump-moka branch with the new solution and it worked. (I am using Erlang/OTP at work, so I had it installed on my Mac)

$ rustc -V           
rustc 1.62.0 (a8314ef7d 2022-06-27)

$ asdf current erlang
erlang          25.0.3          ... /cream/.tool-versions

$ git remote -v
origin	git@github.com:JayKickliter/cream.git (fetch)
origin	git@github.com:JayKickliter/cream.git (push)

$ git status
On branch bump-moka
Your branch is up to date with 'origin/jsk/bump-moka'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   Cargo.lock
	modified:   Cargo.toml

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	.tool-versions

no changes added to commit (use "git add" and/or "git commit -a")

$ git diff Cargo.toml 
diff --git a/Cargo.toml b/Cargo.toml
index 586dd68..7934353 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -10,6 +10,6 @@ name = "cream_nif"
 path = "native/lib.rs"
 
 [dependencies]
-moka = { git = "https://github.com/moka-rs/moka.git", rev = "dcae5d520553b16d49f25922a6b31442aff2a4fa" }
+moka = { git = "https://github.com/moka-rs/moka.git", rev = "865d0899b77f23a9de73accd2c1a4154ac59ffad" }
 rustler = "0.25.0"
 rustler_stored_term = "0.1.0"

$ cargo tree -i moka
moka v0.9.1 (https://github.com/moka-rs/moka.git?rev=865d0899b77f23a9de73accd2c1a4154ac59ffad#865d0899)
└── cream v0.1.0 (... /cream)

$ rebar3 shell    
===> Verifying dependencies...
...
===> Analyzing applications...
===> Compiling cream
Erlang/OTP 25 [erts-13.0.3] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit]

Eshell V13.0.3  (abort with ^G)
1> MaxCapacity = 3.
3

2> {ok, Cache} = cream:new(MaxCapacity).
{ok,#Ref<0.1419974579.4198105090.215813>}

3> ExpensiveOperation = fun() -> io:format("value is not in cache, performing expensive operation\n"), 1 end.
#Fun<erl_eval.43.3316493>

4> 1 = cream:cache(Cache, {my, key}, ExpensiveOperation).
value is not in cache, performing expensive operation
1

5> 1 = cream:cache(Cache, {my, key}, ExpensiveOperation).
1

Copy link
Member Author

@tatsuya6502 tatsuya6502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging.

@tatsuya6502 tatsuya6502 merged commit f6059e0 into master Jul 16, 2022
@tatsuya6502 tatsuya6502 deleted the get-with-slice branch July 16, 2022 13:22
@JayKickliter
Copy link

thanks so much for the quick fix (and for testing my own code)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants